Repository navigation
feat(ogar-oidc): provider-agnostic OIDC ID-token verifier (PR-01) - #367
Conversation
New crate ogar-oidc implementing ogar_auth::federation::AssertionVerifier for compact JWS ID tokens: - ES256 (p256 from the AdaWorldAPI/elliptic-curves fork; ecdsa patched to the AdaWorldAPI/signatures fork) and EdDSA (ogar-encryption). none, HS* and crit are refused; RS256 is not implemented (no RSA fork wired). - Keys are looked up only in the token issuer's own KeySet, so one issuer's key never verifies a token naming another. - iss byte-equal and trusted, sub, aud/azp, exp/iat/nbf with leeway, nonce; strength from amr; email only when email_verified. - No network I/O: Discovery::from_json and KeySet::from_jwks_json take bytes the caller fetched; the clock and nonce come in with the token. 16 integration tests; nine disable runs (key search across issuers, unknown alg defaulting to ES256, no azp rule, no trust check, exp off by one, nonce ignored, unverified email, key type not matched to alg, crit accepted) each turn their test red. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XAqKMQJzQD5KvNm8BtbsWG
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars.
 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e50aae0eb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let distinct = FACTORS.iter().filter(|f| values.contains(f)).count(); | ||
| if distinct >= 2 { | ||
| AuthStrength::MultiFactor |
There was a problem hiding this comment.
A valid token with amr: ["hwk", "user"] is classified as MultiFactor, although it can describe a single hardware-key authentication with a user-presence check. RFC 8176 §2 defines user as evidence of presence, not an independent authentication factor. Similarly, different method labels can describe the same factor. establish copies this result directly into the authenticated user's context, so these logins receive an overstated authentication strength. Infer MFA only from combinations that establish independent factors, or conservatively require an explicit trusted MFA assertion; add a regression test for hardware-key authentication with presence only.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 38c7b37.
MultiFactor now requires either mfa in amr, or amr values that cover two independent factor categories (RFC 8176): knowledge (pwd pin kba), possession (hwk swk otp sms tel pop sc) or inherence (fpt face iris retina vbm). user (presence) and other labels that are not a factor count for nothing.
New cases in amr_decides_strength:
["hwk","user"]→ SingleFactor;["otp","sms"](one category) → SingleFactor;["hwk","fpt"]→ MultiFactor.
Restoring the old rule, where any two labels counted, turns the test red.
Generated by Claude Code
amr ["hwk","user"] described one hardware-key authentication with a presence check, yet counted as two factors. Strength now requires mfa or two of knowledge / possession / inherence; user (presence) is not a factor. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XAqKMQJzQD5KvNm8BtbsWG
|
@codex review Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 38c7b37246
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let Some(key) = parse_jwk(jwk)? else { | ||
| set.skipped += 1; | ||
| continue; | ||
| }; |
There was a problem hiding this comment.
When a JWKS expresses key restrictions through key_ops rather than use, this loader ignores them. For example, a P-256 key with key_ops: ["deriveKey"] and no use or alg is imported as an ES256 verification key, and tokens signed with its private key can pass verify even though the issuer did not authorize that key for signature verification. Check key_ops when present and exclude keys whose permitted operations do not include verify; add a regression test alongside the existing encryption-key filtering test.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 539a7db.
KeySet::from_jwks_json now skips any key whose key_ops is present but does not list verify, and also any key whose key_ops is not an array (RFC 7517 §4.3). It counts each skipped key in skipped().
Two cases were added to jwks_parsing_skips_and_rejects:
- a P-256 key with
key_ops: ["deriveKey"]and nouseis skipped; - a key with
key_ops: ["verify"]is kept.
The test goes red when the check is removed.
Generated by Claude Code
…Codex P2) A key whose key_ops is present but lacks "verify" (or is not an array) is skipped, per RFC 7517 §4.3, instead of being imported as a verification key. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XAqKMQJzQD5KvNm8BtbsWG
|
@codex review Generated by Claude Code |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
What
PR-01 adds the crate
ogar-oidc. It implements theAssertionVerifierinterface from PR-00 (#366) for OIDC ID tokens in compact JWS form.ogar_auth::federation::establishthen turns the claims it returns into aVerifiedLogin.A provider is configured as a
Client: the issuer, the client id, and the issuer's JWKS. No provider has its own code path, so ZITADEL, Keycloak and Entra ID use the same verifier.Discovery::from_json,KeySet::from_jwks_json). Key rotation isClient::set_keys. The current time and the expected nonce arrive with the token (IdToken), so verification is deterministic.none,HS*and any header withcritare refused.useis notsig, or whosekey_opsdoes not listverify, are skipped the same way.issis read before the signature is checked, but only to choose whose keys to try. A key registered for one issuer can never verify a token that names another.issmust byte-equal a registered issuer and be inTrustedIssuers.submust be non-empty.audmust contain the client id. With several audiences,azpis required, and when present it must equal the client id.expandiatare required andnbfis optional, all checked with a leeway (default 60 s).noncemust match when the login sent one.MultiFactorrequires eithermfainamr, oramrvalues covering two independent factor categories: knowledge, possession, inherence (RFC 8176).user(presence) is not a factor. Anything else givesSingleFactor.email_verifiedis true, and only as a display hint.OidcErrorcarries no token contents.Tests
The 16 integration tests are in
crates/ogar-oidc/tests/verifier.rs. They include an end-to-endestablishrun with both a bound and an unbound identity.Disable runs. Each one turns red only the test named next to it:
key_of_one_issuer_never_verifies_anotheralgdefaults to ES256alg_none_hs256_and_crit_are_unsupportedcritacceptedalg_none_hs256_and_crit_are_unsupportedazprule for several audiencesaudience_and_authorized_party_rulesTrustedIssuerscheckissuer_is_byte_exact_and_must_be_trustedexpleeway off by onetime_claims_and_leeway_boundariesnonce_must_match_when_expectedemail_verifiedemail_is_returned_only_when_verifiedalgkid_selection_rulesamrlabels count as MFAamr_decides_strengthkey_opsignoredjwks_parsing_skips_and_rejectsGates:
cargo test -p ogar-oidc -p ogar-auth: green.cargo clippy -p ogar-oidc --all-targets -- -D warnings: clean.cargo fmt --all -- --check: clean.Dependencies (forks first)
p256: AdaWorldAPI/elliptic-curves at rev739304e026fdf06cd1a31606e4db487d3f47c5ae, the same rev ndarray'sencryptioncrate pins.ecdsa0.17.0: redirected by a new workspace[patch.crates-io]to AdaWorldAPI/signatures at rev5595c3b1bfa14022b46ec3cc662a87e49b954266.cargo tree -i ecdsaconfirms the patch is used.ogar-encryption.base64andserde_json: crates.io.docs/UPSTREAM-DEPS.md.Decision: no RS256
The operator ruled to keep the verifier ES256/EdDSA only. A provider must sign ID tokens with a P-256 or Ed25519 key. ZITADEL's default signing key is RSA, so a ZITADEL instance needs an ECDSA or Ed25519 signing key activated.
Correction to PR-00's description
The #366 description said
Issueraccepts http for localhost. It does not:Issuer::newacceptshttps://only, which is whatdocs/AUTH-EXTERNAL-IDENTITY.md§1 states.Not in scope
🤖 Generated with Claude Code
https://claude.ai/code/session_01XAqKMQJzQD5KvNm8BtbsWG